Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Creating ZIP archives with no file extension #21694

Merged
merged 5 commits into from
Dec 3, 2024
Merged

Fix: Creating ZIP archives with no file extension #21694

merged 5 commits into from
Dec 3, 2024

Conversation

lowvoltage
Copy link
Contributor

This fixes #21693

There are a number of changes involved:

  • Add a --adjust-sfx command-line flag when invoking the zip binary -- the actual fix for the bug
  • Modify the test_create_zip_archive unit test:
    • Parametrize it for multiple test scenarios
    • Modify the last section, as it was failing for anything but .zip extensions
    • Note: This is a best-effort change that I'm not super confident in. There may be better or more canonical ways to do this
  • Remove the unsed ZipBinary.create_archive_argv method

@lowvoltage
Copy link
Contributor Author

P.S. Any advice on the failing "Ensure PR has release notes (pull_request)" check?

Error: Please do one of:

- add release notes to the appropriate file in `docs/notes`

- label this PR with `release-notes:not-required` if it does not need them (for
  instance, if this is fixing a minor typo in documentation)

- label this PR with `category:internal` if it's an internal change

@lowvoltage lowvoltage marked this pull request as ready for review November 26, 2024 15:20
@lilatomic lilatomic added needs-cherrypick category:bugfix Bug fixes for released features labels Nov 28, 2024
@lilatomic lilatomic added this to the 2.22.x milestone Nov 28, 2024
@lilatomic
Copy link
Contributor

Looks good. I've marked this bugfix for backporting

To satisfy the "Ensure PR has release notes" check, you'd need to add a changelog to "docs/notes/2.25.x.md". Probably under the "General" heading.

@lilatomic lilatomic self-assigned this Nov 28, 2024
@lowvoltage lowvoltage force-pushed the fix-zip-without-extension branch from 3df6f61 to a49038d Compare November 28, 2024 05:36
@lowvoltage lowvoltage changed the title WIP: Fix: Creating ZIP archives with no file extension Fix: Creating ZIP archives with no file extension Nov 28, 2024
@lowvoltage
Copy link
Contributor Author

Bump ^^
This is ready for a final review

@lilatomic
Copy link
Contributor

Sorry for the delay! Looks good. I wanted to confirm that this wouldn't alter anything for zipfiles with extensions. It would be a shame if we broke reproduceable builds or worse.

I can confirm that zipfiles produced with --adjust-sfx are byte-for-byte equivalent with those produced without it in the case that there is no self-extracting information. zip reports. If you're trying this experiment yourself, beware that creating a zipfile will alter the atime, but not always (probably caching or something).

head -c 10M </dev/urandom >myfile
touch -at 11111111 myfile
zip 0.zip myfile
touch -at 11111111 myfile
zip --adjust-sfx 1 myfile

xxd 0.zip > 0.hex
xxd 1 > 1.hex
diff 0.hex 1.hex

I also checked that most major distros packaged a zip that has this option. Debian, Arch, and Alpine all bundle Info-Zip's implementation, which supports this option. Busybox does not implement zip so we don't have to worry about that.

lilatomic
lilatomic approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@lilatomic lilatomic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@lilatomic lilatomic merged commit e14d5bc into pantsbuild:main Dec 3, 2024
24 checks passed
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.22.x

Successfully opened #21707.

✔️ 2.23.x

Successfully opened #21706.

✔️ 2.24.x

Successfully opened #21705.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

cburroughs added a commit that referenced this pull request Dec 6, 2024
@WorkerPants @lowvoltage @cburroughs
Fix: Creating ZIP archives with no file extension (Cherry-pick of #21694
18036bf 
) (#21705)

This fixes #21693

There are a number of changes involved:
* Add a `--adjust-sfx` command-line flag when invoking the `zip` binary
-- the actual fix for the bug
* Modify the `test_create_zip_archive` unit test:
  * Parametrize it for multiple test scenarios
* Modify the last section, as it was failing for anything but `.zip`
extensions
* **Note**: This is a best-effort change that I'm not super confident
in. There may be better or more canonical ways to do this
* Remove the unsed `ZipBinary.create_archive_argv` method

Co-authored-by: Dimitar Kovachev <[email protected]>
Co-authored-by: cburroughs <[email protected]>
huonw added a commit that referenced this pull request Jan 16, 2025
@WorkerPants @lowvoltage @huonw
Fix: Creating ZIP archives with no file extension (Cherry-pick of #21694
c683e54 
) (#21706)

This fixes #21693

There are a number of changes involved:
* Add a `--adjust-sfx` command-line flag when invoking the `zip` binary
-- the actual fix for the bug
* Modify the `test_create_zip_archive` unit test:
  * Parametrize it for multiple test scenarios
* Modify the last section, as it was failing for anything but `.zip`
extensions
* **Note**: This is a best-effort change that I'm not super confident
in. There may be better or more canonical ways to do this
* Remove the unsed `ZipBinary.create_archive_argv` method

Co-authored-by: Dimitar Kovachev <[email protected]>
Co-authored-by: Huon Wilson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilatomic lilatomic lilatomic approved these changes

Assignees

@lilatomic lilatomic

Labels
category:bugfix Bug fixes for released features
Projects
None yet
Milestone
2.22.x
Development

Successfully merging this pull request may close these issues.

archive target is unable to produce a ZIP file with no extension
3 participants
@lowvoltage @lilatomic @WorkerPants